Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pretty-printing of $crate (take 4) #62393

Merged
merged 4 commits into from
Jul 10, 2019
Merged

Conversation

petrochenkov
Copy link
Contributor

Pretty-print $crate as crate or crate_name in unstructured tokens like a $crate c in foo!(a $crate c), but only if those tokens are printed as a part of AST pretty-printing, rather than as a standalone token stream.

Fixes #62325
Previous iterations - #56647, #57155, #57915.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@bors

This comment has been minimized.

@petrochenkov
Copy link
Contributor Author

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

I think another test here that checks for us doing the right thing with an external crate, i.e., $crate when it resolves to foo vs. crate would be good -- but I'm not sure if we have one already.

With or without that, r=me

...but only if those tokens are printed from inside of AST pretty-printing.
Stop visiting AST to discover those contexts, just iterate through hygiene data instead
@petrochenkov
Copy link
Contributor Author

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jul 9, 2019

📌 Commit 4cb67c0 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 10, 2019
…ulacrum

Fix pretty-printing of `$crate` (take 4)

Pretty-print `$crate` as `crate` or `crate_name` in unstructured tokens like `a $crate c` in `foo!(a $crate c)`, but only if those tokens are printed as a part of AST pretty-printing, rather than as a standalone token stream.

Fixes rust-lang#62325
Previous iterations - rust-lang#56647, rust-lang#57155, rust-lang#57915.
bors added a commit that referenced this pull request Jul 10, 2019
Rollup of 5 pull requests

Successful merges:

 - #61853 (Emit warning when trying to use PGO in conjunction with unwinding on …)
 - #62278 (Add Iterator::partition_in_place() and is_partitioned())
 - #62283 (Target::arch can take more than listed options)
 - #62393 (Fix pretty-printing of `$crate` (take 4))
 - #62474 (Prepare for LLVM 9 update)

Failed merges:

r? @ghost
@bors bors merged commit 4cb67c0 into rust-lang:master Jul 10, 2019
@bors
Copy link
Contributor

bors commented Jul 10, 2019

⌛ Testing commit 4cb67c0 with merge 0324a2b...

@o01eg
Copy link
Contributor

o01eg commented Jul 10, 2019

I've got error with serde_derive after this PR:

error[E0433]: failed to resolve: unresolved import
  --> <::syn::token::Token macros>:37:55
   |
1  | / ( abstract ) => { crate :: token :: Abstract } ; ( as ) => {
2  | | crate :: token :: As } ; ( async ) => { crate :: token :: Async } ; ( auto )
3  | | => { crate :: token :: Auto } ; ( become ) => { crate :: token :: Become } ; (
4  | | box ) => { crate :: token :: Box } ; ( break ) => { crate :: token :: Break }
...  |
37 | | => { crate :: token :: Colon2 } ; ( , ) => { crate :: token :: Comma } ; ( / )
   | |                                                       ^^^^^
   | |                                                       |
   | |                                                       unresolved import
   | |                                                       help: a similar path exists: `syn::token`
...  |
54 | | ) => { crate :: token :: Tilde } ; ( _ ) => { crate :: token :: Underscore } ;
55 | | ( $ ) => { crate :: token :: Dollar } ;
   | |_______________________________________- in this expansion of `Token!`
   |
  ::: serde_derive/src/fragment.rs:58:18
   |
58 |                   <Token![,]>::default().to_tokens(out);
   |                    --------- in this macro invocation

error: aborting due to 12 previous errors

For more information about this error, try `rustc --explain E0433`.
error: Could not compile `serde_derive`.

@petrochenkov
Copy link
Contributor Author

@o01eg
Could you give some minimized / self-contained reproduction?
I'll look what happens.

bors added a commit that referenced this pull request Jul 11, 2019
pretty-print: Do not lose the `$crate` printing flag in `print_tt`

#62393 had this accidental mistake.

Fixes #62562
r? @Mark-Simulacrum
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 1, 2020
expand: Stop using nonterminals for passing tokens to attribute and derive macros

Make one more step towards fully token-based expansion and fix issues described in rust-lang#72545 (comment).

Now `struct S;` is passed to `foo!(struct S;)` and `#[foo] struct S;` in the same way - as a token stream `struct S ;`, rather than a single non-terminal token `NtItem` which is then broken into parts later.

The cost is making pretty-printing of token streams less pretty.
Some of the pretty-printing regressions will be recovered by keeping jointness with each token, which we will need to do anyway.

Unfortunately, this is not exactly the same thing as rust-lang#73102.
One more observable effect is how `$crate` is printed in the attribute input.
Inside `NtItem` was printed as `crate` or `that_crate`, now as a part of a token stream it's printed as `$crate` (there are good reasons for these differences, see rust-lang#62393 and related PRs).
This may break old proc macros (custom derives) written before the main portion of the proc macro API (macros 1.2) was stabilized, those macros did `input.to_string()` and reparsed the result, now that result can contain `$crate` which cannot be reparsed.

So, I think we should do this regardless, but we need to run crater first.
r? @Aaron1011
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
expand: Stop using nonterminals for passing tokens to attribute and derive macros

Make one more step towards fully token-based expansion and fix issues described in rust-lang#72545 (comment).

Now `struct S;` is passed to `foo!(struct S;)` and `#[foo] struct S;` in the same way - as a token stream `struct S ;`, rather than a single non-terminal token `NtItem` which is then broken into parts later.

The cost is making pretty-printing of token streams less pretty.
Some of the pretty-printing regressions will be recovered by keeping jointness with each token, which we will need to do anyway.

Unfortunately, this is not exactly the same thing as rust-lang#73102.
One more observable effect is how `$crate` is printed in the attribute input.
Inside `NtItem` was printed as `crate` or `that_crate`, now as a part of a token stream it's printed as `$crate` (there are good reasons for these differences, see rust-lang#62393 and related PRs).
This may break old proc macros (custom derives) written before the main portion of the proc macro API (macros 1.2) was stabilized, those macros did `input.to_string()` and reparsed the result, now that result can contain `$crate` which cannot be reparsed.

So, I think we should do this regardless, but we need to run crater first.
r? @Aaron1011
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
expand: Stop using nonterminals for passing tokens to attribute and derive macros

Make one more step towards fully token-based expansion and fix issues described in rust-lang#72545 (comment).

Now `struct S;` is passed to `foo!(struct S;)` and `#[foo] struct S;` in the same way - as a token stream `struct S ;`, rather than a single non-terminal token `NtItem` which is then broken into parts later.

The cost is making pretty-printing of token streams less pretty.
Some of the pretty-printing regressions will be recovered by keeping jointness with each token, which we will need to do anyway.

Unfortunately, this is not exactly the same thing as rust-lang#73102.
One more observable effect is how `$crate` is printed in the attribute input.
Inside `NtItem` was printed as `crate` or `that_crate`, now as a part of a token stream it's printed as `$crate` (there are good reasons for these differences, see rust-lang#62393 and related PRs).
This may break old proc macros (custom derives) written before the main portion of the proc macro API (macros 1.2) was stabilized, those macros did `input.to_string()` and reparsed the result, now that result can contain `$crate` which cannot be reparsed.

So, I think we should do this regardless, but we need to run crater first.
r? @Aaron1011
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
expand: Stop using nonterminals for passing tokens to attribute and derive macros

Make one more step towards fully token-based expansion and fix issues described in rust-lang#72545 (comment).

Now `struct S;` is passed to `foo!(struct S;)` and `#[foo] struct S;` in the same way - as a token stream `struct S ;`, rather than a single non-terminal token `NtItem` which is then broken into parts later.

The cost is making pretty-printing of token streams less pretty.
Some of the pretty-printing regressions will be recovered by keeping jointness with each token, which we will need to do anyway.

Unfortunately, this is not exactly the same thing as rust-lang#73102.
One more observable effect is how `$crate` is printed in the attribute input.
Inside `NtItem` was printed as `crate` or `that_crate`, now as a part of a token stream it's printed as `$crate` (there are good reasons for these differences, see rust-lang#62393 and related PRs).
This may break old proc macros (custom derives) written before the main portion of the proc macro API (macros 1.2) was stabilized, those macros did `input.to_string()` and reparsed the result, now that result can contain `$crate` which cannot be reparsed.

So, I think we should do this regardless, but we need to run crater first.
r? @Aaron1011
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2020
expand: Stop using nonterminals for passing tokens to attribute and derive macros

Make one more step towards fully token-based expansion and fix issues described in rust-lang#72545 (comment).

Now `struct S;` is passed to `foo!(struct S;)` and `#[foo] struct S;` in the same way - as a token stream `struct S ;`, rather than a single non-terminal token `NtItem` which is then broken into parts later.

The cost is making pretty-printing of token streams less pretty.
Some of the pretty-printing regressions will be recovered by keeping jointness with each token, which we will need to do anyway.

Unfortunately, this is not exactly the same thing as rust-lang#73102.
One more observable effect is how `$crate` is printed in the attribute input.
Inside `NtItem` was printed as `crate` or `that_crate`, now as a part of a token stream it's printed as `$crate` (there are good reasons for these differences, see rust-lang#62393 and related PRs).
This may break old proc macros (custom derives) written before the main portion of the proc macro API (macros 1.2) was stabilized, those macros did `input.to_string()` and reparsed the result, now that result can contain `$crate` which cannot be reparsed.

So, I think we should do this regardless, but we need to run crater first.
r? @Aaron1011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$crate incorrectly substituted in attribute macro invocation containing bang macro invocation
6 participants